Skip to content

Conversation

@chu11
Copy link
Member

@chu11 chu11 commented May 16, 2023

In #5055 some code that was only supported in Python 3.11 did not get caught by the vermin check, which surprised me. It ended up it was caught in a newer version of vermin.

So lets increase the version and add a novermin comment for one place that needs it. Also add a comment to the place that needs the novermin override.

Problem: vermin did not detect some "obvious" version incompatability
issues.

Update the pre-commit check version of vermin from 1.4.2 to 1.5.1.
Copy link
Contributor

@grondo grondo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! Just one nit in a comment, I don't think tomllib was added until Python 3.11...


import yaml

# tomllib added to standard library in Python 3.10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# tomllib added to standard library in Python 3.10
# tomllib added to standard library in Python 3.11

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i got conflicting information about 3.10 vs 3.11. Seems we got it wrong in this prior commit too :-( 6298a36. But googling it does seem it is 3.11.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, oops!

Problem: tomllib is included on Python 3.11 and newer versions and
is loaded when available.  This can cause a vermin check failure
because the minimum required version for flux-core is 3.6.

Add a "novermin" comment to pass the vermin check.  Add a small comment
to explain code as well.
@chu11 chu11 force-pushed the vermin_version_update branch from 723be9a to 3b9b6e3 Compare May 16, 2023 18:33
@codecov
Copy link

codecov bot commented May 16, 2023

Codecov Report

Merging #5173 (723be9a) into master (5b0897b) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head 723be9a differs from pull request most recent head 3b9b6e3. Consider uploading reports for the commit 3b9b6e3 to get more accurate results

@@            Coverage Diff             @@
##           master    #5173      +/-   ##
==========================================
- Coverage   83.12%   83.10%   -0.02%     
==========================================
  Files         453      453              
  Lines       77856    77856              
==========================================
- Hits        64717    64705      -12     
- Misses      13139    13151      +12     
Impacted Files Coverage Δ
src/bindings/python/flux/util.py 95.21% <100.00%> (ø)

... and 12 files with indirect coverage changes

@chu11
Copy link
Member Author

chu11 commented May 16, 2023

thanks, setting MWP.

@mergify mergify bot merged commit 9cc7782 into flux-framework:master May 16, 2023
@chu11 chu11 deleted the vermin_version_update branch May 16, 2023 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants